[WIP] feat: capture transient values in loop block for closures #1585

ltzmaxwell · 2024-01-25T17:34:09Z

this is a fix to #1135 .

package main

func main() {
        var fns []func()
        for _, v := range []int{1, 2, 3} {
                x := v*100 + v
                fns = append(fns, func() { println(x) })
        }
        for _, fn := range fns {
                fn()
        }
}

As discussed in #1135, the go behaviour (expected) should is:

101
202
303

while gno behavior:

303
303
303

The reason for this is the funcLit inside loop block is not capturing every transient values of v as the loop went on, It only keeps a reference to it. when this funcLit is in execution, the value of v is 3, so the result will always be 303.

This PR provides a solution to capture the transient state as the loop went on, and use these values when the closure is in execution.

The work flow is:

Identify the specific pattern. It's always showing up with a loop block and a nested funcLit.
Record transient values for captured named vars. the transient state of v is recorded as [1, 2, 3];
Use the recorded values to update context. when fn (the closure) is in execution, using [1, 2, 3] to update the context block(in which v == 3) to be 1, 2, 3 , and get the expected results.

Here are some examples to show in detail:

this is generally same with for loops maintain the same block on iteration, which is referenced in any closures generated within #1135, the fix makes it consistent go Go.

package main

// this is a fix, intuitive, and same with Go
func main() {
	var fns []func() int
	for i := 0; i < 5; i++ {
		x := i
		f := func() int {
			return x
		}
		fns = append(fns, f)
	}
	for _, fn := range fns {
		println(fn())
	}
}

// Go Output:
// 0
// 1
// 2
// 3
// 4

// Output:
// 0
// 1
// 2
// 3
// 4

this is a fix to make it consistent with Go's experimental feature of loopVar. It's a design decision if we want this.

package main

// this is a fix, intuitive, same with go loopVar experiment feature.
func main() {
	var fns []func() int
	for i := 0; i < 5; i++ {
		f := func() int {
			return i
		}
		fns = append(fns, f)
	}
	for _, fn := range fns {
		println(fn())
	}
}

// Go Output(without loopVar flag):
// 5
// 5
// 5
// 5
// 5

// Output:
// 0
// 1
// 2
// 3
// 4

this one is consistent with Go too, but from an intuitive perspective, It should capture x the var declared outside of the for block, I'd like to leave it for discussion since it's a design decision.

package main

// this still keeps consistent with go, that a variable out of loop block is not captured
// this is unintuitive(should capture something)
// TODO: leave it for discuss.

func main() {
	var fns []func() int
	var x int
	for i := 0; i < 5; i++ {
		x = i
		f := func() int {
			return x
		}
		fns = append(fns, f)
	}
	for _, fn := range fns {
		println(fn())
	}
}

// Go Output:
// 4
// 4
// 4
// 4
// 4

// Output:
// 4
// 4
// 4
// 4
// 4

codecov · 2024-01-25T17:37:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.75%. Comparing base (6452476) to head (dc61511).
Report is 124 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1585       +/-   ##
===========================================
- Coverage   55.87%   44.75%   -11.13%     
===========================================
  Files         430      438        +8     
  Lines       65618    67250     +1632     
===========================================
- Hits        36667    30100     -6567     
- Misses      26083    34637     +8554     
+ Partials     2868     2513      -355

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jaekwon · 2024-02-01T22:09:53Z

gnovm/pkg/gnolang/op_assign.go

@@ -5,6 +5,7 @@ func (m *Machine) doOpDefine() {
 	// Define each value evaluated for Lhs.
 	// NOTE: PopValues() returns a slice in
 	// forward order, not the usual reverse.
+	// m.PopValue()


deelawn · 2024-02-09T01:36:35Z

@ltzmaxwell amazing! 🤩 Thanks as always for tackling these difficult VM issues.

Some things I really like about this:

it seems like you have a pretty clear understanding of the issue and the approach to fixing it
the added tests are super helpful to understand behavior and guard against future regressions

I spent quite a bit of time reviewing your solution, as well as time trying to understand both the problem a bit more. After thinking about this, I arrived at a related but different way to solve the problem using fewer abstractions. Do you think that you could give it a look? Perhaps there is a combined solution that would work well; the code you wrote here certainly made it easier for me to commit my own thoughts to code. https://github.com/deelawn/gno/pull/3/files

Also, I was thinking about how a similar issue might arise that doesn't use explicit loops. Consider the following example:

package main

func main() {
	var y, counter int
	var f []func()
	defer func() {
		for _, ff := range f {
			ff()
		}
	}()
LABEL:
	if counter == 5 {
		return
	}
	x := y
	f = append(f, func() { println(x) })
	y++
	counter++
	goto LABEL
}

// Output:
// 0
// 1
// 2
// 3
// 4

The solution you've presented here does not account for this scenario. How easy do you think it would be to incorporate it?

I'm currently not set on either solution; there may be an even better way than both of us have thought of. I'm hoping some others can post their thoughts here as well, so let me know what you think 😄

thehowl · 2024-02-15T17:21:09Z

Just a status update: @jaekwon discussed with maxwell for an alternative approach, which fixes this issue in Preprocessing. @ltzmaxwell, please do ping us for a second review when done!

ltzmaxwell · 2024-02-19T08:42:17Z

Just a status update: @jaekwon discussed with maxwell for an alternative approach, which fixes this issue in Preprocessing. @ltzmaxwell, please do ping us for a second review when done!

Make it draft until done.

…ix goto with label case

thehowl · 2024-03-13T18:31:03Z

gnovm/tests/debug2/defer.gno

note: your result doesn't match Go's output: https://go.dev/play/p/pbx4N-F4W89

ltzmaxwell · 2024-04-03T10:49:31Z

house keeping.

ltzmaxwell added 18 commits January 4, 2024 15:16

fixup

4c95054

fixup

1b87465

simplify

26b454e

fixup

8dc512c

fixup

0fd3de3

fixup

0831b1a

fixup

f331607

fixup

6200338

fixup

cf84a19

make test pass

e4ec67b

fixup

c874d1f

fixup

4808321

fixup

d7b913e

range support

36f8345

fix index

3fb64f3

clean

aa542a3

clean

494d998

clean

4cb1710

ltzmaxwell requested review from jaekwon, piux2, thehowl, moul and a team as code owners January 25, 2024 17:34

github-actions bot assigned ltzmaxwell Jan 25, 2024

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jan 25, 2024

ltzmaxwell added 2 commits January 26, 2024 02:04

clean

a33899a

fixup

465bb3b

ltzmaxwell requested a review from a team as a code owner January 26, 2024 00:27

github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jan 26, 2024

ltzmaxwell mentioned this pull request Jan 26, 2024

for loops maintain the same block on iteration, which is referenced in any closures generated within #1135

Closed

fixup

1544e38

ltzmaxwell requested a review from harry-hov as a code owner January 26, 2024 02:16

ltzmaxwell added 5 commits January 26, 2024 14:04

fixup

c57a0f2

fixup

f4e98da

fix lint

1630655

tidy

90c629d

rm unused comment

3af55bb

thehowl requested review from deelawn and mvertes January 31, 2024 10:55

jaekwon reviewed Feb 1, 2024

View reviewed changes

zivkovicmilos mentioned this pull request Feb 12, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

thehowl mentioned this pull request Feb 15, 2024

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

ltzmaxwell marked this pull request as draft February 19, 2024 08:39

ltzmaxwell changed the title ~~feat: capture transient values in loop block for closures~~ [WIP] feat: capture transient values in loop block for closures Feb 19, 2024

ltzmaxwell added 6 commits February 20, 2024 23:21

move closure pattern identification to preprocess as PoC; trying to f…

925e9dd

…ix goto with label case

hacky proprocess

629b9d7

simplify doOpFunc logic

6b29a90

comment & clean unused logic

0e82640

comment

f39c80e

goto, break

dc61511

thehowl reviewed Mar 13, 2024

View reviewed changes

gnovm/tests/debug2/defer.gno

Copy link

Member

thehowl Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: your result doesn't match Go's output: https://go.dev/play/p/pbx4N-F4W89

thehowl mentioned this pull request Mar 13, 2024

fix: re-create blocks on for loop iterations #1768

Closed

jaekwon mentioned this pull request Mar 15, 2024

WIP: inject closure-calls for loop externs in closures #1780

Closed

1 task

ltzmaxwell closed this Apr 3, 2024

ltzmaxwell mentioned this pull request Apr 25, 2024

feat(gnovm): solution for correctly capturing loop externs #1818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: capture transient values in loop block for closures #1585

[WIP] feat: capture transient values in loop block for closures #1585

ltzmaxwell commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading

jaekwon Feb 1, 2024

deelawn commented Feb 9, 2024

thehowl commented Feb 15, 2024

ltzmaxwell commented Feb 19, 2024

thehowl Mar 13, 2024

ltzmaxwell commented Apr 3, 2024

[WIP] feat: capture transient values in loop block for closures #1585

[WIP] feat: capture transient values in loop block for closures #1585

Conversation

ltzmaxwell commented Jan 25, 2024 • edited Loading

codecov bot commented Jan 25, 2024 • edited Loading

Codecov Report

jaekwon Feb 1, 2024

Choose a reason for hiding this comment

deelawn commented Feb 9, 2024

thehowl commented Feb 15, 2024

ltzmaxwell commented Feb 19, 2024

thehowl Mar 13, 2024

Choose a reason for hiding this comment

ltzmaxwell commented Apr 3, 2024

ltzmaxwell commented Jan 25, 2024 •

edited

Loading

codecov bot commented Jan 25, 2024 •

edited

Loading